Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Coverage Report for CI Build 27786832619Coverage decreased (-0.05%) to 98.353%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR adds a new “Full-text options” link to the ThirdIron/BrowZine fulfillment UI so that journal results with BrowZine data can also offer a path back to the source “full record” page.
Changes:
- Pass a
full_record_urlthrough the BrowZine content-loader request so the BrowZine partial can render a “Full-text options” link. - Adjust BrowZine/LibKey link CSS class usage (tests now assert
.libkey-linkrather than.buttonin some cases). - Add a controller test covering the BrowZine route when
full_record_urlis provided.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
app/controllers/thirdiron_controller.rb |
Plumbs full_record_url from request params into the BrowZine view. |
app/views/search/_result_primo.html.erb |
Extracts the Primo “full record” link and passes it toward the BrowZine trigger. |
app/views/search/_trigger_browzine.html.erb |
Appends full_record_url to the BrowZine loader URL. |
app/views/thirdiron/browzine.html.erb |
Renders the new “Full-text options” link alongside the BrowZine link. |
app/views/thirdiron/libkey.html.erb |
Adjusts BrowZine link CSS classes used in the LibKey-rendered HTML. |
test/controllers/thirdiron_controller_test.rb |
Updates selectors for LibKey HTML and adds a BrowZine + full_record_url test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 new issues
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
|
|
||
| parsed.to_s | ||
| rescue URI::InvalidURIError | ||
| nil |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
app/views/search/_trigger_browzine.html.erb:9
data-content-loader-url-valueis currently unquoted. With the newfull_record_urlquery param this attribute value now includes&and multiple=characters, which can lead to invalid HTML and fragile parsing. Quoting the attribute value makes the markup valid and robust.
<% data_url = "/browzine?#{query_params.to_query}" %>
<span class="libkey-container"
data-controller="content-loader"
data-content-loader-url-value=<%= data_url %>>
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
This works as I understand it should, but please checkin with Dave as to whether the button class should be removed and whether the full text link should be in the lib key-actions div.
If you can figure out the different conditions the two browzine link templates come into play some inline comments would be amazing for future us :)
| <%= link_to 'Full-text options', @full_record_url, class: 'button libkey-link', data: { matomo_seen: "Results, Full-text Options Link Seen, Tab: {{getActiveTabName}}", matomo_click: "Results, Full-text Options Link Engaged, Link: {{getElementText}}", content_piece: 'Full-text options' } %> | ||
| <% end %> | ||
|
|
||
| <div class="libkey-actions"> |
There was a problem hiding this comment.
I'm not sure the intent of this libkey-actions div and whether this full text options link belongs inside or outside of it. I know it isn't libkey related, but it's possible that div was named as such as a means to group the links coming out of this partial when the only links were libkey. It may be worth checking in with Dave to confirm if he has input into whether this new link is in that div or not.
There was a problem hiding this comment.
Moving the link outside of this div was a styling workaround, similar to removing the button class. When the Full-text options link is placed inside the div, both it and the Browse journal link render as if they are the first link in the list.
I'll tag Dave on this to see if he has insight on this and the button class issue.
| <% if @libkey[:browzine_link].present? %> | ||
| <div class="libkey-actions"> | ||
| <%= link_to @libkey[:browzine_link][:text], @libkey[:browzine_link][:link], class: 'button libkey-link', data: { matomo_seen: "Results, Browzine Link Seen, Tab: {{getActiveTabName}}", matomo_click: "Results, Browzine Link Engaged, Link: {{getElementText}}", content_piece: @libkey[:browzine_link][:text] } %> | ||
| <%= link_to @libkey[:browzine_link][:text], @libkey[:browzine_link][:link], class: 'libkey-link', data: { matomo_seen: "Results, Browzine Link Seen, Tab: {{getActiveTabName}}", matomo_click: "Results, Browzine Link Engaged, Link: {{getElementText}}", content_piece: @libkey[:browzine_link][:text] } %> |
There was a problem hiding this comment.
I'm not sure why button is being removed? Don't we add it to all the fulfillment links and the css styles them as buttons or not conditionally?
It's unclear to me when this template comes into play versus the same link in the browzine template. I'm concerned whatever condition this is here for should include the new full text options as well?
There was a problem hiding this comment.
It does, but the addition of the Full text options link affects the conditional styling in such a way that Browse journal renders as a primary button rather than the intended underlined styling. The easiest path I could figure to fix that was to remove the button class, but that doesn't necessarily mean it's the best path.
| <% end %> | ||
|
|
||
| <div class="libkey-actions"> | ||
| <%= link_to @browzine[:browzine_link][:text], @browzine[:browzine_link][:link], class: 'libkey-link', data: { matomo_seen: "Results, Browzine Link Seen, Tab: {{getActiveTabName}}", matomo_click: "Results, Browzine Link Engaged, Link: {{getElementText}}", content_piece: @browzine[:browzine_link][:text] } %> |
There was a problem hiding this comment.
I'm not sure why button is being removed? Don't we add it to all the fulfillment links and the css styles them as buttons or not conditionally?
|
@djanelle-mit Could I get your eyes on this PR? Adding the 'Full-text options' link has had some unintentional effects on styling:
I was trying not to mess with the CSS, hence these HTML workarounds. However, I'm curious whether you think this would be worth adjusting the CSS to accommodate the new links. Let me know if I can clarify anything. Thanks! |
djanelle-mit
left a comment
djanelle-mit
left a comment
There was a problem hiding this comment.
I took a look at the build and adjusting the markup here doesn't seem to have any ill effects on the output. I couldn't find any examples of more than one button or broken styling on the deployment.
I think this is good to go if it was solving visual bugs you ran into with the CSS. We can always refactor the CSS after all these rounds of link PRs land, too... that logic seems to be holding for now, but happy to adjust if need be.
Not sure if I'm the only remaining approver here, but consider it approved from my end. I can adjust this if need be.
|
@JPrevost Sorry, I forgot to add the suggested comments before tagging for re-review. Could you let me know if they match your understanding? |
Why these changes are being introduced: UXWS has requested for journal records with Browzine links should also include a 'Full-text options' button that links to the full record. Relevant ticket(s): - [USE-614](https://mitlibraries.atlassian.net/browse/USE-614) How this addresses that need: This adds the 'Full-text options' link. Side effects of this change: - Button class has been removed from certain LibKey links for styling purposes. - New link is rendered outside of libkey-links div for styling purposes. - Added comments clarifying the Browzine/LibKey partials.
Why these changes are being introduced:
UXWS has requested for journal records with
Browzine links should also include a 'Full-text
options' button that links to the full record.
Relevant ticket(s):
How this addresses that need:
This adds the 'Full-text options' link.
Side effects of this change:
None.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing